Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed "Title jumps to the next line, out of the toolbar #508" #509

Merged
merged 11 commits into from
Apr 14, 2017
Merged

Fixed "Title jumps to the next line, out of the toolbar #508" #509

merged 11 commits into from
Apr 14, 2017

Conversation

touficbatache
Copy link
Contributor

@touficbatache touficbatache commented Apr 12, 2017

Hey,
I tried to fix the toolbar problem with adding a flex: 0; style to the .mdc-toolbar__section--align-end class as mentioned in the issue #508.

EDIT

I added a class mdc-toolbar__section--shrink-to-fit that fixes the problem as well as some essential css code. Thanks to @yeelan0319.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@touficbatache
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #509 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #509   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files          45       45           
  Lines        2050     2050           
  Branches      251      251           
=======================================
  Hits         2030     2030           
  Misses         20       20

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8f862...b022af6. Read the comment docs.

@yeelan0319
Copy link
Contributor

Hey @touficbatache ,

mdc-toolbar__section--* set flex:1 because mdc-toolbar is trying to fit the generic needs and we don't want to impose what users put in each sections, so they by default have equally divided width.

I suggested the solution we would like to have for this problem as comment. If you could modify your PR and we would willing to help you merge it. Thanks for filing the PR to solve the issue.

@@ -68,6 +68,7 @@ $mdc-toolbar-mobile-breakpoint: 599px;
&--align-end {
Copy link
Contributor

@yeelan0319 yeelan0319 Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of modifying --align-end directly, we would like to have a mdc-toolbar--section-shrink-to-fit modifier that set flex: none.

In your specific use case, you can do something like:

<section class="mdc-toolbar__section mdc-toolbar__section--align-start">
  <span class="mdc-toolbar__title">This is a very long title</span>
</section>
<section class="mdc-toolbar__section mdc-toolbar__section--align-end mdc-toolbar--section-shrink-to-fit">
  ...
</section>

@@ -68,6 +68,7 @@ $mdc-toolbar-mobile-breakpoint: 599px;
&--align-end {
justify-content: flex-end;
order: 1;
flex: 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to accommodate a super long title, we also need to add:

.mdc-toolbar__section {
  overflow: hidden;
}

.mdc-toolbar__title {
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

Here is a pen for demonstrating the case.

@touficbatache
Copy link
Contributor Author

@yeelan0319 I made the necessary changes but the problem is that TravisCI is failing.

@yeelan0319
Copy link
Contributor

@touficbatache It seems like this what TravisCI is complaining happened in mdc-toolbar.scss:

77:5 ✖ Expected empty line before declaration declaration-empty-line-before

@touficbatache
Copy link
Contributor Author

touficbatache commented Apr 13, 2017 via email

@yeelan0319
Copy link
Contributor

&__title {
    @include mdc-typography(title);

    overflow: hidden;
    text-overflow: ellipsis;
    white-space: nowrap;
    margin: 0;
    line-height: 1.5rem;
  }

You need a empty line between @include mdc-typography(title); and overflow: hidden;

@touficbatache
Copy link
Contributor Author

touficbatache commented Apr 13, 2017 via email

@yeelan0319
Copy link
Contributor

Yep! Exactly what TravisCI complained about in its log :)

@yeelan0319
Copy link
Contributor

Looks good. I will merge it once check passed.

@touficbatache
Copy link
Contributor Author

touficbatache commented Apr 13, 2017 via email

@yeelan0319
Copy link
Contributor

Since we are adding a new modifier class, we should also documented the behavior in following places:

  1. Under "Sections", where we described

Toolbar sections are laid out using flexbox. Each section will take up an equal
amount of space within the toolbar.

I think we could change it to something like

Toolbar sections are laid out using flexbox. Each section will take up an equal
amount of space within the toolbar by default. But you can accommodate very long section by adding mdc-toolbar__section-shrink-to-fit to the rest of the section.

It should help if we add a paragraph of sample code there.

  1. Under "Modifier", where we documented all the modifiers, add mdc-toolbar__section-shrink-to-fit to it.

I think that should be it! Thanks @touficbatache for your help!

@touficbatache
Copy link
Contributor Author

No problem. Happy to help :) Yes, this should be the last commit. Thanks

@@ -139,3 +139,4 @@ The provided modifiers are:
| `mdc-toolbar--fixed` | Make toolbar fixed to top of screen. |
| `mdc-toolbar__section--align-start` | Makes section aligns to the start. |
| `mdc-toolbar__section--align-end` | Makes section aligns to the end. |
| `mdc-toolbar__section--shrink-to-fit`| Makes section take more space. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are setting flex: none, it is actually the opposite.

Makes section takes the width of its content.

@@ -96,7 +96,7 @@ of the toolbar (respectively).
```

Toolbar sections are laid out using flexbox. Each section will take up an equal
amount of space within the toolbar.
amount of space within the toolbar by default. But you can accommodate very long section (very long title) by adding `mdc-toolbar__section--shrink-to-fit` to the rest of the section
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. the rest of the section >> other sections.

  2. Could you also add the following sample code

<div class="mdc-toolbar>
  <div class="mdc-toolbar__row">
      <section class="mdc-toolbar__section mdc-toolbar__section--align-start">
        <span class="mdc-toolbar__title">This is a super super super super long title</span>
      </section>
      <section class="mdc-toolbar__section mdc-toolbar__section--align-end mdc-toolbar__section--shrink-to-fit">
        <a class="material-icons search align-icons" aria-label="Search" alt="Search">search</a>
      </section>
  </div>
</div>

@yeelan0319 yeelan0319 merged commit 130246f into material-components:master Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants